Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove executable bit #180

Merged
merged 1 commit into from
Dec 9, 2015
Merged

Remove executable bit #180

merged 1 commit into from
Dec 9, 2015

Conversation

netheril96
Copy link
Contributor

The source files should not have executable permission set.

@stleary
Copy link
Owner

stleary commented Dec 7, 2015

The files in the pull request do not show any changes, so far as I can tell.
Are you sure the executable bit being set is not an artifact from your Git client?

@netheril96
Copy link
Contributor Author

@stleary

json-java

GitHub finds 14 files with mode changes. Are you on Windows, where the permission system is different from what git expects?

@stleary
Copy link
Owner

stleary commented Dec 8, 2015

Yes, I am on Windows.

@netheril96
Copy link
Contributor Author

@stleary Windows has a different permission system (executable bit is not even a thing there) so you may not find anything different. It surely is different on Unix.

@stleary
Copy link
Owner

stleary commented Dec 8, 2015

Are the permissions breaking anything?

@netheril96
Copy link
Contributor Author

@stleary Not yet. But it is inconsistent and annoying.

@johnjaylward
Copy link
Contributor

@stleary this is what it currently looks like on OS X or linux
execbit

On Unix, this makes the source code executable to the OS. In general this is frowned upon as a security risk

@stleary
Copy link
Owner

stleary commented Dec 8, 2015

@johnjaylward thanks, I think it would be reasonable to have the code be as correct as possible, including the permissions. Are you recommending just making sure the executable bit is cleared for all files?

@johnjaylward
Copy link
Contributor

yeah, that's what the little note that Github is showing you this pull request does:
CDL.java 100755 → 100644
755 is

  • Read set for "User, Group, Other"
  • Write set for "User"
  • Executable bit set for "User, Group, Other"

644 is:

  • Read set for "User, Group, Other"
  • Write set for "User"
  • Executable bit not set

@johnjaylward
Copy link
Contributor

here's a brief overview of the numeric notation github is showing: https://en.wikipedia.org/wiki/File_system_permissions#Numeric_notation

@stleary
Copy link
Owner

stleary commented Dec 8, 2015

@netheril96 OK, seems reasonable, thanks for the suggestion. It would be helpful if you could update the "version" field in the JavaDoc for each file, replacing the date you find there with the current date, using this format: yyyy-mm-dd. The commit comment should be something simple, such as, "Remove executable permission bit from file mode", or whatever you think is appropriate. Thanks, Sean

@netheril96
Copy link
Contributor Author

@stleary Done

stleary added a commit that referenced this pull request Dec 9, 2015
@stleary stleary merged commit 03dd662 into stleary:master Dec 9, 2015
@stleary
Copy link
Owner

stleary commented Dec 9, 2015

Thanks, files have the correct mode bits set now, unit tests pass, will include this in the next release.

@johnjaylward
Copy link
Contributor

Verified the correct permissions are set.

execbit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants